Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add new configure option --with-mixedrefs for mixed pointer mode #359

Merged
merged 1 commit into from
Nov 14, 2020
Merged

Add new configure option --with-mixedrefs for mixed pointer mode #359

merged 1 commit into from
Nov 14, 2020

Conversation

sharon-wang
Copy link
Contributor

@sharon-wang sharon-wang commented Nov 5, 2020 •
edited
Loading

These are the jdk11 extensions changes needed for eclipse-openj9/openj9#8878.

If --with-mixedrefs is specified in the configure command, the 'default' OPENJ9_LIBS_SUBDIR is used for the build. The mxdptrs CMake file corresponding to the build OS is used as the OPENJ9_BUILDSPEC. --with-mixedrefs=[static/dynamic] sets the reference determination mode. --with-mixedrefs without any value provided defaults to static mode.

The compressed and full GC libraries generated with the mixed build work are copied to the OPENJ9_LIBS_SUBDIR alongside the other existing libraries.

  • j9gc29, j9gcchk29 are always copied
  • j9gc_full29, j9gcchk_full29 are copied when running in mixedrefs static mode

Note that this will only work with an OpenJ9 CMake build. OpenJ9 UMA changes will be implemented after the CMake work is completed and any corresponding OMR/extensions changes will be provided in future PRs.

These changes depend on the new build specs defined in OpenJ9.

OpenJ9 PR: eclipse-openj9/openj9#11166
OMR PR: eclipse-omr/omr#5657

Ports

@sharon-wang
Copy link
Contributor Author

Please review @gacholio

@sharon-wang sharon-wang force-pushed the mixedbuild branch 2 times, most recently from 3f4876f to bc373c3 Compare November 5, 2020 19:45
keithc-ca
keithc-ca requested changes Nov 5, 2020
View reviewed changes
keithc-ca
keithc-ca reviewed Nov 5, 2020
View reviewed changes
@keithc-ca
Copy link
Member

Note that this will only work with an OpenJ9 CMake

The configure script should complain (and fail) if you try to combine --without-cmake and --with-mixedrefs.

@sharon-wang
Copy link
Contributor Author

Thanks @keithc-ca!

Added a check so that configure errors and quits if mixedrefs is used while CMake is not enabled. It's in the CMake configure function since it runs after the platform setup, so at that point we'll know if mixedrefs is set and whether CMake is enabled or not.

openj9-openjdk-jdk11/closed/autoconf/custom-hook.m4

Lines 75 to 82 in b944640

# at this point with_cmake should either be no, or the name of the cmake command
if test "x$with_cmake" = xno ; then
OPENJ9_ENABLE_CMAKE=false
# Currently, mixedrefs mode is only available with CMake enabled
if test "x$OPENJ9_ENABLE_MIXED_REFERENCES" = xtrue ; then
AC_MSG_ERROR([--with-mixedrefs cannot be used if CMake is disabled])
fi

For setting OPENJ9_BUILDSPEC, I've split the build spec into the OS portion OPENJ9_BUILD_OS/OPENJDK_BUILD_OS and the mode/arch portion OPENJ9_BUILD_MODE_ARCH. I concatenate the build OS and the build mode/arch after doing all the tests to construct OPENJ9_BUILDSPEC. Let me know what you think of this approach.

@keithc-ca
Copy link
Member

I think this is ready to merge: please squash.

Will you be preparing pull requests for the other extension repositories?

will be opening a PR with these changes soon

When should we expect that?

@sharon-wang
Copy link
Contributor Author

I think the focus is on JDK11 right now, but let me confirm in the main issue. I think this depends on the availability of CMake in JDK versions/various platforms.

For the openj9 PR, I'm working on finishing up the pipeline changes needed (and testing them) and reorganizing the commits since there's a lot of changes. I expect to open up a PR by mid next week.

I've opened up a PR eclipse-omr/omr#5657 for the OMR GC changes.

@sharon-wang
Copy link
Contributor Author

I've squashed the commits.

Are there documentation changes that are needed with this new configure option in the extensions repo? Or just within openj9/openj9-docs?

@keithc-ca keithc-ca mentioned this pull request Nov 6, 2020
Open
@keithc-ca
Copy link
Member

This new option would be documented in the build instructions for Java 11, and similarly for other versions, probably near places where --with-noncompressedrefs is mentioned.

@gacholio
Copy link
Member

gacholio commented Nov 9, 2020

Would it be worth splitting the mixed refs and dual GC compile options?

@keithc-ca
Copy link
Member

Would it be worth splitting the mixed refs and dual GC compile options?

I don't understand what that means. As I understand it, mixed refs implies dual GC compiles. What could be split?

@gacholio
Copy link
Member

gacholio commented Nov 9, 2020

Mixed refs work fine with the single compile of the GC, it just doesn't perform well. In the future, we may want to invest some time in templating, etc. to allow the single compile to work efficiently.

@sharon-wang
Copy link
Contributor Author

For splitting, are you referring to adding pointer mode checks?

@keithc-ca
Copy link
Member

If supporting both flavours of mixed refs isn't too onerous, it probably makes sense to do that now. I suggest we expose that option via using value of --with-mixedrefs=value to identify the desired mode:

  • blank or 'no' - mixed references are not enabled
  • 'yes' or 'static' - mixed references are enabled using dual compiles
  • 'dynamic' - mixed references are enabled with dynamic (runtime) tests

The values 'static' and 'dynamic' are just my suggestions: others may have better suggestions.

@sharon-wang
Copy link
Contributor Author

Ah I see. So in 'dynamic' mode, we actually don't want to define OMR_OVERRIDE_COMPRESS_OBJECT_REFERENCES.

I can set a new CMAKE argument -DMIXEDREFS_MODE=[static/dynamic], which is read by the CMAKE files in omr to conditionally set the OVERRIDE value.

@gacholio
Copy link
Member

gacholio commented Nov 9, 2020

Correct, dynamic mode just defines both the compressed and full defines, without the override.

keithc-ca
keithc-ca reviewed Nov 10, 2020
View reviewed changes
@sharon-wang
Copy link
Contributor Author

I've added OMR_MIXED_REFERENCES_MODE which is off by default, but can be set to static or dynamic if --with-mixedrefs=[static/dynamic] is used. --with-mixedrefs defaults to static. This will be documented in OpenJ9 doc/build-instructions/Build_Instructions_V[8/11/15/...].md

If we are in mixed mode and static is the setting, then we will set OMR_OVERRIDE_COMPRESS_OBJECT_REFERENCES in OMR.

If these changes look good, I'll port them to the JDK8/15/next equivalents.

keithc-ca
keithc-ca requested changes Nov 11, 2020
View reviewed changes
@keithc-ca keithc-ca mentioned this pull request Nov 11, 2020
Merged
@sharon-wang
Copy link
Contributor Author

I've added the requested changes and updated the description to note:

  • j9gc29, j9gcchk29 are always copied
  • j9gc_full29, j9gcchk_full29 are copied when running in mixedrefs static mode

keithc-ca
keithc-ca requested changes Nov 12, 2020
View reviewed changes
Copy link
Member

@keithc-ca keithc-ca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not as part of this pull request, but at some point I think we should have just one configuration option that affects reference representation (not both --with-mixedrefs and --with-noncompressedrefs), perhaps --with-references=[full|compressed-static|compressed-dynamic] so we don't have to consider precedence of the two options.

@sharon-wang
Add new configure option --with-mixedrefs for mixed pointer mode
33c02cd 
If --with-mixedrefs is specified in the configure command, the 'default'
OPENJ9_LIBS_SUBDIR is used for the build. The mxdptrs CMake file corresponding
to the build OS is used as the OPENJ9_BUILDSPEC. --with-mixedrefs=[static/dynamic]
sets the reference determination mode. --with-mixedrefs without any value provided
defaults to static mode.

OMR_MIXED_REFERENCES_MODE is communicated to OpenJ9 and OMR through CMake args.

Fail configure if --with-mixedrefs is used without CMake enabled.

The new mxdptr build specs are defined in OpenJ9.

The compressed and full GC libraries generated with the mixed build work are
copied to the OPENJ9_LIBS_SUBDIR alongside the other existing libraries.
    j9gc29, j9gcchk29 are always copied
    j9gc_full29, j9gcchk_full29 are copied when running in mixedrefs static mode

If an OpenJ9-equivalent OS has been specified in OPENJ9_BUILD_OS,
use the specified OS string to override the provided
OPENJDK_BUILD_OS.

Use OPENJ9_BUILD_MODE_ARCH to capture the other half of the
OPENJ9_BUILDSPEC, where the architecture and the pointer mode
(or other additional modes/settings) are specified.

Signed-off-by: Sharon Wang <[email protected]>
@sharon-wang
Copy link
Contributor Author

Updated. Help string is now build mixedrefs vm (--with-mixedrefs=static or --with-mixedrefs=dynamic)

For --with-references=[full|compressed-static|compressed-dynamic], do you mean mixed-static|mixed-dynamic?

@gacholio
Copy link
Member

Updated. Help string is now build mixedrefs vm (--with-mixedrefs=static or --with-mixedrefs=dynamic)

For --with-references=[full|compressed-static|compressed-dynamic], do you mean mixed-static|mixed-dynamic?

We'll need [full|compressed|mixed-static|mixed-dynamic]

@sharon-wang sharon-wang mentioned this pull request Nov 12, 2020
Open
@sharon-wang
Copy link
Contributor Author

I opened #361 to keep note of this new configure option.

@sharon-wang sharon-wang mentioned this pull request Nov 12, 2020
Merged
5 tasks
@keithc-ca keithc-ca self-assigned this Nov 12, 2020
@keithc-ca
Copy link
Member

What changes, required by this, are yet to be merged into openj9 and/or omr?

gacholio
gacholio approved these changes Nov 12, 2020
View reviewed changes
Copy link
Member

@gacholio gacholio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll defer to Keith here.

@sharon-wang
Copy link
Contributor Author

@keithc-ca Here are the corresponding PRs

This was referenced Nov 12, 2020
Merged
Merged
Merged
@keithc-ca
Copy link
Member

Jenkins compile aix,osx,win,plinux jdk11

@keithc-ca keithc-ca merged commit a26b9f0 into ibmruntimes:openj9 Nov 14, 2020
@sharon-wang sharon-wang mentioned this pull request Dec 12, 2020
Closed
keithc-ca added a commit to keithc-ca/openj9-openjdk11 that referenced this pull request Mar 24, 2021
@keithc-ca
Merge pull request ibmruntimes#359 from vsebe/ibm-jdk11-jcldev.jzos
390b4f2 
JZOS Integration
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keithc-ca keithc-ca keithc-ca approved these changes

@gacholio gacholio gacholio approved these changes

Assignees

@keithc-ca keithc-ca

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sharon-wang @keithc-ca @gacholio